Repository navigation
Conversation
The stream destroyer (a for await loop that breaks, a pipeline() that fails) destroys a request and keeps its connection. _destroy cleared the body callback of the native handle, so the rest of the body arrived with no callback and req.complete stayed false. Node's parser completes the message at its last chunk. _destroy now leaves a callback armed for such a request. It drops the chunks and sets complete at the last one.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review. WalkthroughWhen an incoming request is destroyed before its body ends, the handler can continue tracking body data if the request has no socket and remains incomplete. Tests cover when the body completes and when the client disconnects before it ends. ChangesRequest body completion
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The change preserves request completion tracking after stream destruction without marking truncated bodies complete. No concrete merge-blocking risk remains; run normal checks before merging. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
StatusReproduced on a release build of main bf42a52. The script in the Notes of the description prints Run: This is the first of two pull requests. #44310 is based on this branch. |
There was a problem hiding this comment.
Beyond the inline findings, I also checked the new callback's wiring: the (chunk, isLast, event) argument shape matches what the native handle already passes to onDataIncomingMessage in src/js/internal/http.ts, and the runtime destructure of the const enum NodeHTTPResponseAbortEvent from require("internal/http") follows the same pattern src/js/node/_http_server.ts already uses, so neither is a concern.
Extended reasoning...
The change alters IncomingMessage.prototype._destroy in src/js/node/_http_incoming.ts to keep a native ondata callback armed after the stream destroyer nulls req.socket, and adds six node:http tests; it touches no auth, crypto, or injection surface. Not approving because verified findings are being posted inline (plus one held back), including a behavioral cost on the early-abandoned-body path and a trailers discrepancy that a maintainer of the node:http server should weigh.
Findings marked 🟡 are optional suggestions and need no follow-up push.
|
Closing: #44310 carries this change now. It is one pull request against main with both changes. |
Part of #43455 (flow 9).
Problem
node:httprequest that the stream destroyer destroyed keepsreq.complete === falseafter its whole body arrived. Node v26.3.0 reportstrue. Afor awaitloop that breaks and a failedpipeline()destroy a request this way.IncomingMessage.prototype._destroy(src/js/node/_http_incoming.ts:371). It clearshandle.ondata, and only that callback setscomplete.Fix
_destroyleaves a callback armed when the connection outlives the request and the message is not complete. The callback drops the chunks and setscompleteat the last one.parserOnMessageCompletein Node setscompletefor a destroyed request too. A connection that closes before the body ends leaves it false.test/js/node/http/node-http-req-complete.test.ts, 6 new cases, 4 fail without the fix. The file also runs under Node.Background
src/js/internal/streams/destroy.ts) setsreq.socket = nulland callsdestroy()._destroythen keeps the connection for the response.handle.ondatais the callback that the native server calls for each chunk of a request body. A flag marks the last chunk.req.complete. Considered adestroyedtest there: it hides the wrong value from one reader only.Downsides
_destroy.server.unref()in the listener, the process stays about 3 s, as in Node. main exits at once.Notes
Repro. The listener breaks out of a
for awaitloop over the request after the first chunk and answers. The client then sends the rest of the body and one more request.Node v26.3.0 prints
true. main bf42a52 printsfalse. This branch printstrue.Rows. 2 ways to destroy the request (
for awaitbreak, failedpipeline()) by 2 framings (Content-Length, chunked):req.completeafter the body ended istruein Node and on this branch for all 4, andfalseon main for all 4. While the body still arrives, and when the client closes the connection before the body ends, it isfalseeverywhere.What Node does.
parserOnMessageComplete(_http_common.js) setsstream.complete = trueand pushes the end of the stream. It does not look atdestroyed.parserOnBodydrops the chunks of a destroyed stream.Why the callback tests the event. The native server also calls
ondatawith the last-chunk flag when the connection closes (abort event). That call must not setcomplete. The twostays falsetests cover it.A body larger than the stream buffer. Node stops reading such a connection when the request is destroyed and never answers a later request on it (a 4 MiB body, then a second request: Node gets no response in 8 s). Bun keeps reading, as before this change, and answers.
req.completeof the destroyed request is thentrue.Measurements. Release builds, Linux x64.
perfandvalgrindare not installed on the machine, so the instruction count for each request is missing._destroy, request that ended normallyjfalseon a local)BUN_JSC_dumpGeneratedBytecodes=1, diff of the listings_destroy, whole function_http_incoming.jswc -c.textllvm-size -Aserver.unref()in the listener, body of the destroyed request outstanding,keepAliveTimeout2 sSuites. Debug build:
node-http-req-complete.test.ts(33 pass). Release builds of main and of #44310, which holds this change, one after the other: see the Notes of #44310.Observed on Linux x64 only.